Add resolve support in refine imports by merging it with explicit imports #3729
Conversation
joyfulmantis
changed the title
joyfulmantis
requested review from
Anton-Latukha,
Ailrun,
michaelpj and
pepeiborra
as code owners
joyfulmantis
changed the title
plugins/hls-explicit-imports-plugin/src/Ide/Plugin/ExplicitImports.hs
Outdated
Show resolved
Hide resolved
| @@ -110,28 +110,31 @@ runImportCommand recorder ideState eird@(ResolveOne _ _) = pluginResponse $ do | |||
| logWith recorder Error (LogWAEResponseError re) | |||
| pure () | |||
| logErrors (Right _) = pure () | |||
| runImportCommand _ _ (ResolveAll _) = do | |||
| pure $ Left $ ResponseError (InR ErrorCodes_InvalidParams) "Unexpected argument for command handler: ResolveAll" Nothing | |||
| runImportCommand _ _ rd = do | |||
There was a problem hiding this comment.
It's weird that we don't have RefineOne, especially since we have the per-import lens, but I guess we didn't before either.
plugins/hls-import-actions-plugin/src/Ide/Plugin/ImportActions.hs
Outdated
Show resolved
Hide resolved
plugins/hls-import-actions-plugin/src/Ide/Plugin/ImportActions.hs
Outdated
Show resolved
Hide resolved
plugins/hls-import-actions-plugin/src/Ide/Plugin/ImportActions.hs
Outdated
Show resolved
Hide resolved
| mkCodeAction "Make this import explicit" (Just $ A.toJSON $ ResolveOne uri int) | ||
| pure $ InL ((InR . toCodeAction _uri <$> relevantCodeActions) <> allExplicit) | ||
| toCodeAction uri (ImportAction _ int RefineImport) = | ||
| mkCodeAction "Refine this import" (Just $ A.toJSON $ ResolveOne uri int) |
There was a problem hiding this comment.
This seems wrong, it's also producing ResolveOne, shouldn't it be RefineOne or sometihng?
| codeActionResolveProvider _ ideState _ ca _ rd = | ||
| pluginResponse $ do | ||
| wedit <- resolveWTextEdit ideState rd | ||
| pure $ ca & L.edit ?~ wedit | ||
| -------------------------------------------------------------------------------- | ||
|
|
||
| resolveWTextEdit :: IdeState -> EIResolveData -> ExceptT String (LspT Config IO) WorkspaceEdit | ||
| resolveWTextEdit :: IdeState -> IAResolveData -> ExceptT String (LspT Config IO) WorkspaceEdit | ||
| resolveWTextEdit ideState (ResolveOne uri int) = do |
There was a problem hiding this comment.
TBH this ResolveOne name is weird. Shouldn't it be ExplicitOne?
There was a problem hiding this comment.
Ugh, I figured it out. We are resolving individual edits generically, using the same machinery for both the explicit import edits and the refine import edits.
That makes me think that we could take this faster. What if we had
data ResolveData = ResolveData { uri :: Uri, importEdits :: [Int] }
Now we can have a totally generic resolve handler: get all the referenced edits, and stick them together into a workspace edit, return. It's just up to the original code action provider to select the edits it will want eventually.
There was a problem hiding this comment.
(the fact that I had to think so much about it tells me that you need to write more comments :) you know this, the reader won't!)
There was a problem hiding this comment.
We need to know the type when we offer code actions or resolve because we have slightly different behavior between the two (titles are different for example).
plugins/hls-import-actions-plugin/src/Ide/Plugin/ImportActions.hs
Outdated
Show resolved
Hide resolved
Yep, no changes there. |
|
Looks like some actual failures in the test suite?? |
Yeah. There was a module name qualifier in a testdata dependent file I forgot to change. Funnily enough, GHC compile rules changed with 9.4+, because they had no problem with the test even though one of the dependent files shouldn't have been able to typecheck... |
No description provided.